Skip to content

Add fallible version of the digital traits and deprecate the current ones#108

Merged
bors[bot] merged 3 commits into
rust-embedded:masterfrom
eldruin:add-fallible-digital-traits
Dec 11, 2018
Merged

Add fallible version of the digital traits and deprecate the current ones#108
bors[bot] merged 3 commits into
rust-embedded:masterfrom
eldruin:add-fallible-digital-traits

Conversation

@eldruin

@eldruin eldruin commented Oct 28, 2018

Copy link
Copy Markdown
Member

There seems to be an agreement in #100 on how to proceed with the fallible traits.
This adds the fallible traits under digital::v2 and marks the current ones as deprecated.
This solves #95

therealprof
therealprof previously approved these changes Oct 28, 2018

@therealprof therealprof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks very much for keeping at it.

@therealprof

Copy link
Copy Markdown
Contributor

@hannobraun @ryankurte Are you okay with this?

@therealprof

Copy link
Copy Markdown
Contributor

@rust-embedded/hal Any other opinions?

@hannobraun hannobraun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Sorry, accidentally submitted this comment early. I edited it to complete it.)

It's possible to add additional information to #[deprecated], namely the version since when something has been deprecated, and a note that is going to be part of the compiler warning (I think). This is documented here: https://doc.rust-lang.org/reference/attributes.html

It would be nice to have this, but I'm fine merging this pull request without.

hannobraun
hannobraun previously approved these changes Oct 28, 2018

@hannobraun hannobraun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant to approve this pull request, but accidentally submitted my comment before it was complete. See my other comment for a note on a nice-to-have item.

Thank you @eldruin for sticking with this! I know working on this project is not easy :)

@eldruin eldruin dismissed stale reviews from hannobraun and therealprof via 421d11b October 28, 2018 15:30
hannobraun
hannobraun previously approved these changes Oct 28, 2018

@hannobraun hannobraun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you again, @eldruin! You're really going above and beyond what can reasonably be expected. I really appreciate it!

ryankurte
ryankurte previously approved these changes Oct 28, 2018

@ryankurte ryankurte left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me! Thanks again @eldruin for working with us through all this.

We don't currently have an adaptor between v1 and v2, do we want to provide that? (I suggest as a separate PR if so).

@caemor

caemor commented Nov 3, 2018

Copy link
Copy Markdown

Is there anything missing here?

@ryankurte

Copy link
Copy Markdown
Contributor

We still need adapter traits (see #100 for examples) before we release, but I'll merge this for now.

bors r+

@bors

bors Bot commented Nov 13, 2018

Copy link
Copy Markdown
Contributor

👎 Rejected by label

@ryankurte

Copy link
Copy Markdown
Contributor

Oops, @therealprof are you still happy with this?

@therealprof

Copy link
Copy Markdown
Contributor

@ryankurte Yepp, I'm good. Needs conflict resolution, though.

therealprof
therealprof previously approved these changes Nov 13, 2018
@ryankurte ryankurte dismissed stale reviews from therealprof, hannobraun, and themself via 14cac99 November 28, 2018 00:35
@ryankurte

Copy link
Copy Markdown
Contributor

bors r+

@bors

bors Bot commented Nov 28, 2018

Copy link
Copy Markdown
Contributor

👎 Rejected by too few approved reviews

@ryankurte

Copy link
Copy Markdown
Contributor

whoops

bors r+

bors Bot added a commit that referenced this pull request Nov 28, 2018
108: Add fallible version of the digital traits and deprecate the current ones r=ryankurte a=eldruin

There seems to be an agreement in [#100](#100 (comment)) on how to proceed with the fallible traits.
This adds the fallible traits under `digital::v2` and marks the current ones as deprecated.

Co-authored-by: Diego Barrios Romero <eldruin@gmail.com>
Co-authored-by: Ryan <ryankurte@users.noreply.github.com>
@bors

bors Bot commented Nov 28, 2018

Copy link
Copy Markdown
Contributor

Build failed

@therealprof

Copy link
Copy Markdown
Contributor

Funny complaints from the nightly compiler. 👎

@rust-embedded/resources Should we make the nightly compiler builds informal only, i.e. not fail CI? Our main target is beta/stable anyways.

@caemor

caemor commented Nov 28, 2018

Copy link
Copy Markdown

cargo test is only run on nightly atm.

This might be because two of the doc tests rely on nightly features (https://github.com/rust-embedded/embedded-hal/blob/master/src/lib.rs#L385 ++ and https://github.com/rust-embedded/embedded-hal/blob/master/src/lib.rs#L557 ++). Maybe the doc test for these two should be tagged with the ignore setting until these features are stable/beta.

but either way the failed doc test doesn't have anything to do with this PR.

@ryankurte

Copy link
Copy Markdown
Contributor

bors try

bors Bot added a commit that referenced this pull request Dec 11, 2018
@bors

bors Bot commented Dec 11, 2018

Copy link
Copy Markdown
Contributor

try

Build succeeded

@ryankurte

Copy link
Copy Markdown
Contributor

bors r+

bors Bot added a commit that referenced this pull request Dec 11, 2018
108: Add fallible version of the digital traits and deprecate the current ones r=ryankurte a=eldruin

There seems to be an agreement in [#100](#100 (comment)) on how to proceed with the fallible traits.
This adds the fallible traits under `digital::v2` and marks the current ones as deprecated.

Co-authored-by: Diego Barrios Romero <eldruin@gmail.com>
Co-authored-by: Ryan <ryankurte@users.noreply.github.com>
@bors

bors Bot commented Dec 11, 2018

Copy link
Copy Markdown
Contributor

Build succeeded

@bors bors Bot merged commit 14cac99 into rust-embedded:master Dec 11, 2018
@eldruin eldruin deleted the add-fallible-digital-traits branch December 12, 2018 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants